[SC-17187] ZD-717: GINITable fails on multiclass models (follow-up to PR #535)#539
Conversation
GINITable computed AUC/GINI/KS with binary-only sklearn calls against a single positive-class probability column, raising "multiclass format is not supported" on models with more than two classes. Add a one-vs-rest path that reads the full per-class probability matrix from the underlying estimator's predict_proba, one-hot encodes labels with label_binarize, and returns one row per class plus a micro-average. Binary behavior is unchanged; models without a usable predict_proba are skipped with SkipTestError instead of crashing. Add multiclass unit tests covering the per-class table and the skip path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pull requests must include at least one of the required labels: |
PR SummaryThis PR extends the functionality of the GINITable module by adding support for multiclass classification metrics. The key changes include:
Overall, these changes improve the library's capability to handle multiclass model evaluations while maintaining backward compatibility with binary classification scenarios. Test Suggestions
|
hunner
left a comment
There was a problem hiding this comment.
Approving — the fix is correct and directly resolves the remaining failure in ZD-717 (GINITable on multiclass models). Verified locally on the PR branch: all 6 unit tests pass (4 existing binary + 2 new multiclass), and I probed the eval-dataset-missing-a-training-class case — the shape guard raises a clear SkipTestError rather than silently mislabeling curves. Binary path is unchanged, and the implementation faithfully mirrors #535's _multiclass_roc_curve.
Left three non-blocking suggestions inline (shared helper extraction, model.classes_ alignment, and one extra test) — all fine as follow-ups.
| ) | ||
|
|
||
|
|
||
| def _multiclass_gini_table( |
There was a problem hiding this comment.
Non-blocking / follow-up: this preamble (raw-estimator access → SkipTestError fallbacks → shape check → label_binarize) is now the 5th copy of the same block, alongside ROCCurve, PrecisionRecallCurve, ConfusionMatrix, and PopulationStabilityIndex from #535. The repo already extracts shared helpers for this kind of thing (see 0a80b55 "share diagnosis metric helpers") — a get_multiclass_proba_matrix(model, dataset, classes) helper would collapse all five call sites and give one place to fix alignment/skip behavior. Keeping this PR consistent with #535 and extracting in a follow-up across all five seems right.
| - The test does not incorporate a method to efficiently handle missing or inefficiently processed data, which could | ||
| lead to inaccuracies in the metrics if the data is not appropriately preprocessed. | ||
| """ | ||
| classes = np.unique(dataset.y) |
There was a problem hiding this comment.
Non-blocking / follow-up: np.unique(dataset.y) is assumed to match the predict_proba column order, but sklearn orders columns by estimator.classes_ — the classes seen in training. For the ZD-717 customer's exact usage (input_grid over [vm_train_ds, vm_test_ds]), a rare class absent from the test split makes this skip on test while succeeding on train (I reproduced this: 4-class model, 3-class eval slice → SkipTestError from the shape guard). Safe, but a false negative — aligning on getattr(raw_model, "classes_", None) with np.unique as fallback would still compute in that case. Same inherited behavior in the four #535 tests, so best fixed once in the shared helper.
| self.assertEqual(set(raw_data.fpr), {"0", "1", "2", "micro"}) | ||
| self.assertEqual(set(raw_data.tpr), {"0", "1", "2", "micro"}) | ||
|
|
||
| def test_multiclass_without_predict_proba_is_skipped(self): |
There was a problem hiding this comment.
Non-blocking: there's coverage for predict_proba being unavailable, but not for the other skip branch — a probability matrix whose column count doesn't match the dataset's classes (e.g. model trained on 4 classes, eval dataset containing only 3). That's the branch multiclass users are most likely to hit in practice via train/test input_grid, and it's cheap to add: fit on 4 classes, build the VM dataset from a slice with y != 3, assert SkipTestError.
…(ZD-717) Follow-ups from the PR #539 review. ROCCurve, PrecisionRecallCurve, PopulationStabilityIndex (#535) and GINITable (#539) each carried an identical ~40-line preamble to fetch the underlying estimator's per-class probability matrix for their one-vs-rest multiclass paths. Extract it into sklearn/_multiclass_proba.py (same pattern as _diagnosis_metrics.py) and point all four tests at it. Fix the class/column alignment while there: the preamble assumed np.unique(dataset.y) matches predict_proba column order, but sklearn orders columns by estimator.classes_ (training classes). Align on classes_ with a np.unique fallback, binarize against the full training class list, and emit per-class output only for classes present in the evaluated y. Net behavior change: an evaluation slice missing a training class now computes metrics for the present classes plus micro-average instead of raising SkipTestError. Two guards the old width check provided implicitly are kept explicit: a binary-trained model evaluated on a >2-class dataset skips (previously the new alignment would have crashed with IndexError), and evaluation labels the model was never trained on skip rather than being silently absorbed as all-negative rows. Adds unit tests for the helper (10) and un-gated sklearn multiclass tests for all four call sites, including the missing-class scenario; the pre-existing multiclass tests for ROC/PR were entirely xgboost-gated, so the refactored paths now have coverage without the extra. PSI gains its first unit test file. 114 passed / 5 xgboost-gated skips across tests/unit_tests/model_validation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pull Request Description
What and why?
GINITable(validmind/tests/model_validation/statsmodels/GINITable.py) computed AUC/GINI/KS with binary-only sklearn calls (roc_curve,roc_auc_score) against a single positive-class probability column. On a model with more than two classes it raisedValueError: multiclass format is not supported.This adds a one-vs-rest path for multiclass models, mirroring the approach PR #535 used for the sklearn tests (ROCCurve, PrecisionRecallCurve, ConfusionMatrix, PopulationStabilityIndex). #535 fixed those four but did not touch
statsmodels.GINITable; this is the remaining follow-up.After this change:
micro-average row (with aClasscolumn). Per-class probabilities are read from the underlying estimator'spredict_proba, since theVMModelwrapper exposes only the positive-class column.SkipTestErrorinstead of crashing.How to test
Run the existing unit tests:
Or reproduce the original failure and confirm the fix directly (this is the snippet from the repro notebook used to validate the change):
Expected multiclass output:
What needs special review?
The multiclass path reads probabilities from the underlying estimator (
model.model.predict_proba) rather than theVMModelwrapper'sy_prob, because the wrapper returns only the positive-class column. Worth confirming this is the right access pattern (it matchesROCCurvefrom #535) and that theSkipTestErrorfallback covers the model types you expect (metadata-only / precomputed single-column probabilities).Dependencies, breaking changes, and deployment notes
Follow-up to #535. No breaking changes — binary output is unchanged.
Release notes
GINITable now supports multiclass classification models. For models with more than two classes it reports AUC, GINI, and KS one-vs-rest (one row per class plus a micro-average) instead of failing with
multiclass format is not supported. Binary models are unaffected.Checklist